Usg msg reason #275
Usg msg reason #275
Conversation
There was a problem hiding this comment.
Thanks for the pull request! ❤️
Comments marked as ❗️ are blocking. The other ones are more of comments and suggestions.
The two suggested Enum.join(invalid, ", ") occurrences may be DRIED into a function.
defp join_for_usage_message(args):
Enum.join(args, ", ")
Co-authored-by: Glutexo <[email protected]>
Co-authored-by: Glutexo <[email protected]>
test/onigumo_cli_test.exs
Outdated
| assert usage_message_printed?(fn -> Onigumo.CLI.main(unquote(combination)) end) | ||
| assert usage_message_printed?( | ||
| fn -> Onigumo.CLI.main(unquote(combination)) end, | ||
| # there is no easy way to do it dynamically |
There was a problem hiding this comment.
The constant can be amended to contain the expected output.
@invalid_combinations [
{["--help", "-C", ".", "downloader"], ["--help"],
{["downloader", "-h"], ["--help"]},
{["downloader", "--help"], ["--help"]},
]--help is the only incompatible option though. That means the second element is unnecessary.
We can however omit the comment: there is an easy way to do it, it’s just not necessary.
| # there is no easy way to do it dynamically |
| # there is no easy way to do it dynamically | |
| # The incompatible argument is always --help. |
Another thing came to my mind, however. It’s how we always call help to be the the incompatible one and never --working-dir. It makes sense to some extent: a component defines a set of valid switches containing --working-dir and not --help. Thus, it’s --help that’s incompatible. But it may become more complicated if the CLI becomes more convoluted.
There was a problem hiding this comment.
I pick Request changes for only two reasons:
- The
# there is no easy way to do it dynamicallycomment doesn’t feel appropriate. There is and easy way. Either do it, change the comment to be more descriptive, or omit it at all. - The
@invalid_switchesextension deserves a preliminary PR that would unblock this one.
test/onigumo_cli_test.exs
Outdated
| assert usage_message_printed?(fn -> Onigumo.CLI.main(unquote(combination)) end) | ||
| assert usage_message_printed?( | ||
| fn -> Onigumo.CLI.main(unquote(combination)) end, | ||
| # there is no easy way to do it dynamically |
There was a problem hiding this comment.
The constant can be amended to contain the expected output.
@invalid_combinations [
{["--help", "-C", ".", "downloader"], ["--help"],
{["downloader", "-h"], ["--help"]},
{["downloader", "--help"], ["--help"]},
]--help is the only incompatible option though. That means the second element is unnecessary.
We can however omit the comment: there is an easy way to do it, it’s just not necessary.
| # there is no easy way to do it dynamically |
| # there is no easy way to do it dynamically | |
| # The incompatible argument is always --help. |
Another thing came to my mind, however. It’s how we always call help to be the the incompatible one and never --working-dir. It makes sense to some extent: a component defines a set of valid switches containing --working-dir and not --help. Thus, it’s --help that’s incompatible. But it may become more complicated if the CLI becomes more convoluted.
| assert usage_message_printed?( | ||
| fn -> Onigumo.CLI.main(unquote(combination)) end, | ||
| # there is no easy way to do it dynamically | ||
| "incompatible OPTIONS --help" |
There was a problem hiding this comment.
| "incompatible OPTIONS --help" | |
| "incompatible OPTIONS #{invalid}" |
If the constant contained tuples as imagined above, the string would look like this.
| "-c" | ||
| ["--invalid"], | ||
| ["-c"], | ||
| ["-a", "-b"] |
There was a problem hiding this comment.
🤔 I am not sure about this. It adds the possibility to test joining for the usage message. But this test should have been there in the first place. Or at least list of invalid switches instead of a single value. I feel it doesn’t belong into this PR.
However, without the list support and this test in place, it is not possible to test the new feature.
There was a problem hiding this comment.
I would keep it maybe in different format, but you have to test a new feature in this PR
There was a problem hiding this comment.
I see a way how to solve this:
Remove the join both from the executive code and from the tests. That is, only report the first invalid switch. It’s a common fail-fast approach that is not that bad.
| ["-a", "-b"] |
Then, create an issue to report all invalid arguments, not only the first one. For that a PR would add this missing piece.
An alternative approach is to create a new PR before hat. It would change the parametrization to lists and possibly add the new extra item. Merging that PR would unlock this one.
| "-c" | ||
| ["--invalid"], | ||
| ["-c"], | ||
| ["-a", "-b"] |
There was a problem hiding this comment.
I see a way how to solve this:
Remove the join both from the executive code and from the tests. That is, only report the first invalid switch. It’s a common fail-fast approach that is not that bad.
| ["-a", "-b"] |
Then, create an issue to report all invalid arguments, not only the first one. For that a PR would add this missing piece.
An alternative approach is to create a new PR before hat. It would change the parametrization to lists and possibly add the new extra item. Merging that PR would unlock this one.
| ["--invalid"], | ||
| ["-c"], |
There was a problem hiding this comment.
| ["--invalid"], | |
| ["-c"], | |
| "--invalid", | |
| "-c" |
| assert usage_message_printed?(fn -> Onigumo.CLI.main([unquote(switch)]) end) | ||
| assert usage_message_printed?( | ||
| fn -> Onigumo.CLI.main(unquote(switch)) end, | ||
| "invalid OPTIONS #{Enum.join(unquote(switch), ", ")}" |
There was a problem hiding this comment.
| "invalid OPTIONS #{Enum.join(unquote(switch), ", ")}" | |
| "invalid OPTION #{unquote(switch)}" |
fix #251